feat(eslint): ban dynamic imports in test hooks, and convert the last 33 sites - #3026
Merged
Merged
Conversation
… 33 sites The prose rule added in #3015 did not hold on its own, and there is hard evidence for that: sweeping for `beforeAll(async () => { await import(…) })` found 37 files, and EVERY one already carried a raised timeout, on an escalating ladder — 15s -> 30s -> 60s. One even left the escalation as advice (`// Increase timeout to 30 seconds for heavy renderer imports`), and plugin-kanban blew its raised 15s anyway at 15021ms (#3010). A convention that gets re-broken 37 times is a lint rule. Adds `object-ui/no-dynamic-import-in-test-hook` (error, scoped to test files): a module loaded inside `beforeAll`/`beforeEach` bills its cold Vite transform to `hookTimeout`, so the test passes or fails on machine load rather than on the code it covers. Imported at module scope the same cost lands in the import phase, which no test or hook timeout applies to. Kept deliberately narrow, with both exemptions pinned in the RuleTester because both are legitimate and both are real code here: - a lazy FACTORY — `registerLazy('x', () => import('./x'))` — the hook installs a loader, it never runs the import. The hook frame resets inside a nested function, so this never reports. (Without this the rule would flag the console's own plugin registration and be unusable.) - a deliberate re-import against per-run module state (`vi.resetModules`, `doMock`, `doUnmock`, `stubEnv`, `stubGlobal`) — there the re-import is the point, and hoisting it would break the test rather than speed it up. No autofixer on purpose: the mechanical case is a one-line move, but the general case captures the module into a variable the tests read, and hoisting that means introducing top-level await and reordering side effects. Because an `error` ratchet has to lint clean today (the convention the other three rules follow), this also converts the 33 remaining sites — 30 in packages/components, 3 in plugin-dashboard. 27 were the canonical pure warm-up. The other 6 also register overrides that must run AFTER the barrel, so only the import moved out; those keep a (now sync, now untimed) hook, and the ordering still holds because static imports are evaluated before any hook. The side-effect import goes after the last existing import — the faithful equivalent of a hook that ran once every static import had been evaluated. Verified: - rule: 19 RuleTester cases (12 valid / 7 invalid); a fresh violation fails lint as an error with the exact fix in the message. - repo sweep: 33 violations -> 0. The only remaining error-level lint findings are 3 pre-existing ones under `e2e/`, which CI's per-package `turbo run lint` does not cover; untouched here. - tests: the 33 converted files pass (209 tests), and the full suite is 100% green — 733 files passed / 1 skipped, 8552 tests passed / 24 skipped. - the suite's cumulative timed portion dropped 243s -> 127s. Wall clock is unchanged (~229s): the work did not disappear, it moved out of timed windows into the import phase, which is the point. AGENTS.md §9 now points at the rule instead of only describing the convention, including both exemptions so the next agent does not fight the linter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The prose rule added in #3015 did not hold on its own — and there is hard evidence for that. Sweeping for
beforeAll(async () => { await import(…) })found 37 files, and every single one already carried a raised timeout, on an escalating ladder:One of them even left the escalation as advice —
// Increase timeout to 30 seconds for heavy renderer imports— andplugin-kanbanblew its raised 15s anyway at 15021ms (#3010). A convention that gets re-broken 37 times is a lint rule, not a paragraph.The rule
object-ui/no-dynamic-import-in-test-hook(error, scoped to test files). A module loaded insidebeforeAll/beforeEachbills its cold Vite transform tohookTimeout, so the test passes or fails on machine load rather than on the code it covers. At module scope the same cost lands in the import phase, which no test or hook timeout applies to.The message names the exact fix rather than just complaining:
Kept narrow — two exemptions, both pinned in the RuleTester
Both are legitimate, and both are real code in this repo, so neither is left to judgement:
registerLazy('x', () => import('./x')). The hook installs a loader; it never runs the import. The hook frame resets inside a nested function, so this never reports. Without this the rule would flag the console's own plugin registration and be unusable.vi.resetModules()/doMock/doUnmock/stubEnv/stubGlobalin the same hook. There the re-import is the point, and hoisting it would break the test rather than speed it up.No autofixer, on purpose. The mechanical case is a one-line move, but the general case captures the module into a variable the tests read — hoisting that means introducing top-level await and reordering side effects, which is not a rewrite a fixer should make unattended.
Why the 33 conversions are in the same PR
An
errorratchet has to lint clean today — that is the convention the repo's other three rules follow ("Error so a new violation fails CI; the existing sites were converted first"). Awarnwould have enforced nothing here, since this repo already tolerates hundreds of warnings. So the rule only actually gates if the backlog goes with it.The side-effect import goes after the last existing import, which is the faithful equivalent of a hook that ran once every static import had already been evaluated. (I initially placed it after the first import and caught two bugs in review: one spliced into the middle of a multi-line
import { … } from, and one reordered evaluation. Both fixed before anything was committed.)Verification
e2e/, which CI's per-packageturbo run lintdoes not cover; untouched here.AGENTS.md §9 now points at the rule instead of only describing the convention, and documents both exemptions so the next agent doesn't fight the linter.
Test/lint infrastructure only — no shipped code changes, so no changeset.
🤖 Generated with Claude Code